Skip to content

Refactor string interpolators and error message handling #16384

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Nov 24, 2022

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Nov 20, 2022

The idea is that we want to concentrate error handling logic in the Message type instead of dispersing it over various
string interpolators.

  • Pass more messages instead of strings to report methods.
  • Make this easier by having string interpolators that produce messages
  • Use messages instead of strings elsewhere
  • Move logic for limiting size of messages and filtering out nonsensical errors into Message.

From the doc comment for Message:

Tips for error message generation

  • You can use the em interpolator for error messages. It's defined in core.Decorators.
  • You can also use a simple string argument for error or warning (not for the other variants),
    but the string should not be interpolated or composed of objects that require a
    Context for evaluation.
  • When embedding interpolated substrings defined elsewhere in error messages,
    use i and make sure they are defined as def's instead of vals. That way, the
    possibly expensive interpolation will performed only in the case where the message
    is eventually printed. Note: At least during typer, it's common for messages
    to be discarded without being printed. Also, by making them defs, you ensure that
    they will be evaluated in the Message context, which makes formatting safer
    and more robust.
  • For common messages, or messages that might require explanation, prefer defining
    a new Message in file messages.scala and use that instead. The advantage is that these
    messages have unique IDs that can be referenced elsewhere.

@odersky odersky changed the title Refactor messages Refactor: avoid passing strings to report.error/warning Nov 20, 2022
@odersky odersky changed the title Refactor: avoid passing strings to report.error/warning Refactor string interpolators Nov 20, 2022
@odersky odersky changed the title Refactor string interpolators Refactor string interpolators and error message handling Nov 20, 2022
 - Revise string interpolators; `em` and `exm` nor produce
   messages, whereas `i`, `e`, and `ex` produce strings.
 - Use Message intead of () => String in some places.
Let them take Messages instead of Strings.

We now are in a situation where the `e` interpolator is only used within
`messages` itself. This means we can hopefully roll its behavior into the
`Message` logic.
This will be the basis on which we modify contexts for printing
diagnsotics.
@odersky odersky force-pushed the refactor-messages branch 3 times, most recently from 4ca606a to e64014f Compare November 21, 2022 19:30
In particular: The missingArg message of ImplicitSearchError becomes a
regular Message now.
That leaves just two interpolators:

 - i   for strings
 - em  for error messages (and other diagnostics)
Also: use exclusively Message insteax of String for error
handling in Scanner and Parser.
Using Message instead of String for arguments of report methods.
String versions are still retained for external interop. But they
should be used only if the argument is a simple string literal.
@odersky odersky marked this pull request as ready for review November 24, 2022 11:58
 - Use aggregation instead of inheritance
 - Encapsulate explanations inside Seen
@odersky
Copy link
Contributor Author

odersky commented Nov 24, 2022

Who would like to give this a review? To be sure, given the change set a lof of it will have to be cursory.

Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been reading the diffs as you've been pushing them. The only thing I didn't reread when code moved files, so I missed any changes in there (if any). But the rest LGTM.

@odersky
Copy link
Contributor Author

odersky commented Nov 24, 2022

OK, great! The code that moved from Formatting to Message was unchanged except for straightforward refactorings.

@odersky odersky merged commit e25362d into scala:main Nov 24, 2022
@odersky odersky deleted the refactor-messages branch November 24, 2022 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants